Support for named profiles (RFC 2678) #6989

da-x · 2019-05-27T18:53:00Z

Tracking issue: #6988

Implementation according to the RFC.

This allows creating custom profiles that inherit from other profiles. For example, one can have a release-lto profile that looks like this: [profile.release-lto] inherits = "release" lto = true The profile name will also carry itself into the output directory name so that the different build outputs can be cached independently from one another. So in effect, at the `target` directory, a name will be created for the new profile, in addition to the 'debug' and 'release' builds: ``` $ cargo build --profile release-lto $ ls -l target debug release release-lto ```

rust-highfive · 2019-05-27T18:53:04Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

ehuss · 2019-05-31T23:12:03Z

Nice! I'm looking forward to landing this.

Did you have any questions or have anything you want me to look at now? I've only skimmed this so far, and the only major thing that jumped out is that all new behavior will need to be behind a feature gate (documented in features.rs).

da-x · 2019-06-01T10:29:55Z

Should the feature gate only affect the use of the [profile.<custom-name>] clauses, or should it also make other behavior changes conditional? (There are other changes, such as the added consistency for smoothing out of the behavior of specifying --profile= combined with --debug and --release across command).

When resolving profile parameters for custom named profiles, we can skip 'dev' and 'release' because they are the root profiles from which all others inherit. Otherwise, for example, we fail the tests where a simple `[profile.dev]` clause is specified.

da-x · 2019-06-01T11:10:02Z

Pushed changes: fixed a bug ; fixed the build of Cargo's tests ; fixed two tests, there are 25 tests still failing which need fixing.

The predefined 'test' and 'bench' profiles should exist even if the user did not define anything in the Cargo.toml. This also rewrites how the merging is done.

The profile returned for custom build should have the proper 'root' field value. This fixes test build_script::profile_and_opt_level_set_correctly.

Since we also for 'cargo rustc' to received '--profile check', let it be consistent with the other commands and use a predefined 'check' profile for it that inherits from "dev". Fixes tests check::rustc_check and check::rustc_check_err.

da-x · 2019-06-01T12:29:04Z

More fixes made, 12 failures remaining.

da-x · 2019-06-01T13:25:27Z

@ehuss The profile_targets::profile_selection* test failures are mainly the ones that may benefit from your observation. The implementation changed which profile is being picked for which target. This is some of the confusion we wanted to clear up regarding how Cargo picks from the various profiles under various commands.

The results from the current PR changes are that at least for profile_selection_bench(), only codegen-units=4 is being seen in verbose, which means only [profile.bench] gets picked up, which I assume is what we wanted. The effects mainly stem from the removal of the reliance over CompileMode:: in src/cargo/core/profiles.rs.

ehuss · 2019-06-03T23:13:51Z

I think the feature gate should cover the new [profile] tables and the --profile flag. Generally anything that changes the UI. The --profile flag can just be gated on -Zunstable-options (just check config.cli_unstable().unstable_options before reading it).

As for the change in consistency for test/bench, I'm on the fence. I think it would be OK to change it now as long as it inherits the settings from dev/release. Assuming that, then I think you should go ahead and update the profile_selection tests.

Before the RFC, the 'clean' command would have cleaned entirety of target/ unless `--release` is specified - in that case it would have cleaned only `target/release`. Initial commit broke that. Now, passing `--profile=NAME` will clean the directory related to profile `NAME` via the dir-name setting. `--release` is supported for backward compatibility.

New code relies on a BTreeMap, rather than handed coded calls.

… on it

ehuss · 2019-09-21T23:43:14Z

Not sure if you saw my question about check, fix, and rustc above. Can those support named profiles?

I'll try to get back to you on the mode issue, I'd like to talk to the rest of the team about it.

da-x · 2019-09-22T06:30:02Z

fix and check - both make use of --profile to determine if it's "test" or not. If we remove that code, it will make use of the custom profile, but then we would still need to determine the boolean given to CompileMode::Check in some other way. One way to solve it is check if we indirectly or directly inherit from test profile. Another is to add a new attribute to all profiles that would be used to determine CompileMode only for those special commands, and then lift the restriction regarding "test".
rustc already makes use of --profile with the changes, and also uses it to determined CompileMode, and likely needs a similar solution for it.

bors · 2019-09-25T16:33:15Z

☔ The latest upstream changes (presumably #7421) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2019-09-26T17:48:28Z

☔ The latest upstream changes (presumably #7425) made this pull request unmergeable. Please resolve the merge conflicts.

ehuss · 2019-09-27T17:15:07Z

We discussed the issues with how targets relate to profiles (such as in --all-targets), and…we didn't really have a good answer. There was some desire to keep the existing behavior where targets can drive which profile is chosen (maybe even making that explicit or configurable). But we decided this is something we can iterate on later, and we'd definitely like to get this PR landed.

The main issue left is that I don't think there should be any changes to existing behavior without the use of unstable flags. Can you go through and make sure the original behavior is retained unless cargo-features = ["named-profiles"] is specified? The ones I see are:

All the changes in testsuite/profile_targets.rs should keep the original behavior.
It looks like profile inheritance is happening for stable profiles. That is, a profile like test is inheriting dev. That should only happen with the unstable feature flag. It should retain the original behavior of using the hard-coded defaults for each profile unless the cargo-feature is specified.

The effects over the profile used by targets are made conditional in this commit, using the old scheme if the `named-profiles` feature is disabled. This also affects the `profile_targets` tests, which now have two modes - stable, and nightly with the feature enabled.

…e=check` And a small cleanup in `base_profile` to make the logic parallel to the one in `get_profile`.

da-x · 2019-09-28T00:36:48Z

Done changes to keep original behavior, except regarding the finished line print which would take a bit more effort (and some wide-reaching test result parameterization - I am not sure if it's wanted).

ehuss · 2019-09-30T19:51:15Z

Thanks so much! This looks great!

I'll try to follow up on the tracking issue with some more things to discuss.

@bors r+

bors · 2019-09-30T19:51:16Z

📌 Commit fad192d has been approved by ehuss

bors · 2019-09-30T19:51:24Z

⌛ Testing commit fad192d with merge 8b0561d...

Support for named profiles (RFC 2678) Tracking issue: #6988 Implementation according to the [RFC](https://github.com/rust-lang/rfcs/blob/master/text/2678-named-custom-cargo-profiles.md).

bors · 2019-09-30T20:56:04Z

☀️ Test successful - checks-azure
Approved by: ehuss
Pushing 8b0561d to master...

da-x · 2019-10-10T06:55:45Z

@ehuss, previously misguided attempts to affect debug builds via [profile.debug], which resulted in an unused key warning in Cargo, now fail via features.require(Feature::named_profiles()). Are we keeping this behavior or should we support backward compatibility until the feature is ungated?

ehuss · 2019-10-10T17:00:56Z

unused key warning in Cargo

Hm, so to be clear, it now issues an error if you don't have the feature enabled and when you do enable the feature, it will require an inherits field, but still issues a warning?

I think I'm fine with that for now. It was an invalid configuration before (it was ignored other than the warning). I don't think we need to be backwards compatible with all invalid configurations.

We may want to reconsider the warning in the future, but doesn't seem too important.

rust-highfive assigned ehuss May 27, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 27, 2019

da-x mentioned this pull request May 27, 2019

Tracking issue for custom-named-profile (RFC 2678) #6988

Closed

13 tasks

ehuss mentioned this pull request May 28, 2019

--debug/--release should be accepted by run/build/install/test/... #6992

Closed

da-x added 4 commits June 1, 2019 13:44

tests: fix and extend config_load_toml_profile for RFC 2689

55f4b67

tests: we now allow a profile.debug

a72c405

tests: profile_doc_deprecated should have 'inherits'

7c6a3d4

da-x added 4 commits June 1, 2019 15:06

custom-named-profiles: fix merges of predefined profiles

b52cc1b

The predefined 'test' and 'bench' profiles should exist even if the user did not define anything in the Cargo.toml. This also rewrites how the merging is done.

tests: config_load_toml_profile fixup

bb86a5c

custom-named-profiles: fix profile for custom build

92339a5

The profile returned for custom build should have the proper 'root' field value. This fixes test build_script::profile_and_opt_level_set_correctly.

custom-named-profiles: predefined check profile

c27d899

Since we also for 'cargo rustc' to received '--profile check', let it be consistent with the other commands and use a predefined 'check' profile for it that inherits from "dev". Fixes tests check::rustc_check and check::rustc_check_err.

ehuss added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 5, 2019

da-x added 6 commits June 7, 2019 21:47

custom-named-profiles: for 'cargo test' use "test" profile by default

e37253c

tests: adjust profiles::profile_panic_test_bench for sorted output

a0290b1

New code relies on a BTreeMap, rather than handed coded calls.

tests: fix finished line for 'cargo test'

e25f6a4

tests: adjust profile selection according to new code

913b69c

Merge remote-tracking branch 'origin/master' into custom-profile-pr-rfc

5d048e0

ehuss mentioned this pull request Jun 7, 2019

Use a single profile set per workspace #3249

Merged

tests: fix finished line for 3 other tests

b0db193

da-x added 3 commits September 21, 2019 10:29

toml: add missing profile name validation

cdd85cc

tests: profile_custom - add name validation tests

3307176

toml: be less stringent on 'debug' profile, we already emit a warning…

a08a1a3

… on it

profiles: remove 'check' from stable access

b437782

Merge remote-tracking branch 'origin/master' into custom-profile-pr-rfc

33d5c83

ehuss mentioned this pull request Sep 26, 2019

Support [profile.*] for setting things across all profiles #7439

Closed

Merge remote-tracking branch 'origin/master' into custom-profile-pr-rfc

04804ea

ehuss mentioned this pull request Sep 26, 2019

Tracking issue for RFC 2282 - Cargo profile dependencies rust-lang/rust#48683

Closed

5 tasks

da-x added 2 commits September 27, 2019 23:50

named-profiles: fix backward compatibility for rustc with `--profil…

fad192d

…e=check` And a small cleanup in `base_profile` to make the logic parallel to the one in `get_profile`.

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Sep 30, 2019

bors merged commit fad192d into rust-lang:master Sep 30, 2019

bors mentioned this pull request Sep 30, 2019

Stabilize cache-messages #7450

Merged

r3v2d0g mentioned this pull request Oct 5, 2019

Fixed building on nightly bastion-rs/bastion#34

Merged

ordian mentioned this pull request Oct 30, 2019

Snapshot restoration overhaul openethereum/parity-ethereum#11219

Merged

ehuss added this to the 1.40.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for named profiles (RFC 2678) #6989

Support for named profiles (RFC 2678) #6989

da-x commented May 27, 2019 •

edited

Loading

rust-highfive commented May 27, 2019

ehuss commented May 31, 2019

da-x commented Jun 1, 2019

da-x commented Jun 1, 2019

da-x commented Jun 1, 2019

da-x commented Jun 1, 2019

ehuss commented Jun 3, 2019

ehuss commented Sep 21, 2019

da-x commented Sep 22, 2019

bors commented Sep 25, 2019

bors commented Sep 26, 2019

ehuss commented Sep 27, 2019

da-x commented Sep 28, 2019

ehuss commented Sep 30, 2019

bors commented Sep 30, 2019

bors commented Sep 30, 2019

bors commented Sep 30, 2019

da-x commented Oct 10, 2019

ehuss commented Oct 10, 2019

Support for named profiles (RFC 2678) #6989

Support for named profiles (RFC 2678) #6989

Conversation

da-x commented May 27, 2019 • edited Loading

rust-highfive commented May 27, 2019

ehuss commented May 31, 2019

da-x commented Jun 1, 2019

da-x commented Jun 1, 2019

da-x commented Jun 1, 2019

da-x commented Jun 1, 2019

ehuss commented Jun 3, 2019

ehuss commented Sep 21, 2019

da-x commented Sep 22, 2019

bors commented Sep 25, 2019

bors commented Sep 26, 2019

ehuss commented Sep 27, 2019

da-x commented Sep 28, 2019

ehuss commented Sep 30, 2019

bors commented Sep 30, 2019

bors commented Sep 30, 2019

bors commented Sep 30, 2019

da-x commented Oct 10, 2019

ehuss commented Oct 10, 2019

da-x commented May 27, 2019 •

edited

Loading